Switch our string interpolators to use Show/Shown #14455

dwijnand · 2022-02-11T16:12:04Z

As it was our string interpolators were taking Any values and then
trying to pattern match back their classes to try to show them nicely.
This inevitably fails for things like the opaque type FlagSet, which
interpolates as an indecipherable long number.

Now, instead, they take "Shown" arguments, for which there is an
implicit conversion in scope, given there is a Show instance for value.
I captured some desired results in some new unit test cases.

In the process a small handful of bugs were discovered, the only
particularly bad one was consuming a Iterator when the "transforms"
printer was enabled (accessorDefs), followed by an unintentional
eta-expansion of a method with a defaulted argument (showSummary).

I also lifted out the Showable and exception fallback function as an
extension method, so I could use it more broadly.

The use of WrappedResult and its result in showing was also
impacted, because the new expected Shown type was driving result's
context lookup. Fortunately I was able to continue to use WrappedResult
and result as defined by handling this API change inside showing
alone.

I wasn't, however, able to find a solution to the impact the new Shown
expected type was having on the stripModuleClassSuffix extension
method, sadly.

JSExportsGen is interpolating a private type, so rather than open its
access or giving it a Show instance I changed the string interpolation.

SyntaxFormatter was no longer used, since the "hl" interpolator was
dropped.

[test_windows_full]

smarter · 2022-02-17T12:51:19Z

I'm lacking time to review this properly, could you find someone else to take over? Overall this looks cool but I'm a bit concerned that any time I want to print some case class or enum I'll need to add boilerplate (maybe all Products should be showable by default?). Also having all three of Show, Shown and Showable seems like a lot to keep track of in one codebase.

dwijnand · 2022-02-17T13:29:14Z

Overall this looks cool but I'm a bit concerned that any time I want to print some case class or enum I'll need to add boilerplate (maybe all Products should be showable by default?).

Hmm, yeah, good point, perhaps Products can be like Showable and automatically accepted. That would probably cull down a bunch of default instances that delegate to toString, too.

Also having all three of Show, Shown and Showable seems like a lot to keep track of in one codebase.

Shown is the mechanical one, just to have an expected type for the interpolators methods to direct the implicit conversion, so it's just private to Formatting and Decorators. Then there's the status quo Showable and the combining Show typeclass. So I don't think we can lose Shown, but I could look to dropping Showable if desired.

dwijnand · 2022-02-23T23:00:51Z

@nicolasstucki Had to rebase to fix a merge conflict (from the unused import cleanup). Any chance you could review this, please?

som-snytt · 2022-02-24T00:08:03Z

Had to rebase to fix a merge conflict (from the unused import cleanup).

Thanks, that's how you know it's working.

compiler/src/dotty/tools/dotc/printing/Formatting.scala

compiler/src/dotty/tools/dotc/core/Decorators.scala

som-snytt · 2022-02-24T04:14:55Z

This requires more than a glance.

I see that the use case is opaques like FlagSet, so pick a better toString based on that type, instead of a pattern-matching printer.

I'm suspicious of having to say i"${thing.show}". Why isn't there:

    object Shown:
      given [A: Show]: Conversion[A, Shown] = Show[A].show(_)
      given fallback[A](using NotGiven[Show[A]]): Conversion[A, Shown] = ShowAny.show

That is as far as I got.

I haven't yet sorted the oddity of an argument conversion (which may be perfectly natural, according to recent discussion about marking a conversion into the desired type), a conversion that is Any => Any albeit with a marker, the Show business (oops, pun) that is not a typeclass, in particular that everything "is a" Showable and implements

def toText(printer: Printer): Text = printer.toText(this)

so the question about whether to just require a Context to print anything seems germane: to print my Foo, shouldn't I just supply a context with a specialized FooPrinter?

It feels like there is an extra degree of indirection from Any -> Shown -> Show -> Showable -> Printer. If I make a Foo, I'm happy to follow the instructions and also make a FooPrinter so it looks nice in debug output. I guess also in error messages. I think I saw on a ticket that those are different use cases.

My other unbaked thought was can't a macro just look at what you're trying to format and summon the appropriate toStringer.

nicolasstucki · 2022-02-24T08:02:45Z

My other unbaked thought was can't a macro just look at what you're trying to format and summon the appropriate toStringer.

We should avoid the use of macros in the compiler. That could make bootstrapping much harder.

compiler/src/dotty/tools/dotc/reporting/messages.scala

compiler/src/dotty/tools/dotc/core/Decorators.scala

nicolasstucki · 2022-02-24T08:41:04Z

compiler/src/dotty/tools/dotc/printing/Formatting.scala

-        }
-      case _ => String.valueOf(arg)
-    }
+    protected def showArg(arg: Any)(using Context): String = arg.show


Is this still necessary? Aren't all arguments already shown by doing the implicit conversion to Shown?

It's necessary for the wrapNonSensical in ErrorMessageFormatter. But it's a part that I haven't explored much. ~~And I'm also kind of ignoring the errorMessageCtx part of that override too...~~

~~But we definitely should be able to change this to arg.toString.~~

Oh yeah, now I remember better: yes this is necessary. ShowAny just returns it's argument, it doesn't call toString. So then showArg calls show, which means the right context will be used, including ErrorMessageFormatter case.

I tried to convey that in the scaladoc. Do you think it needs changing to convey this better? And/or should I add something to showArg?

/** The base implementation, passing the argument to StringFormatter which will try to `.show` it. */ object ShowAny extends Show[Any]: def show(x: Any): Shown = x

dwijnand · 2022-02-24T11:53:40Z

Thanks for the review, @som-snytt!

I see that the use case is opaques like FlagSet, so pick a better toString based on that type, instead of a pattern-matching printer.

I'm suspicious of having to say i"${thing.show}". Why isn't there:
    object Shown:
      given [A: Show]: Conversion[A, Shown] = Show[A].show(_)
      given fallback[A](using NotGiven[Show[A]]): Conversion[A, Shown] = ShowAny.show
That is as far as I got.

Well, for one, because I forgot about NotGiven. 😄 But now that I think about it, it trades off having to "deal with" adding this support to your new type with not catching bugs like trying to toString a lambda or an interator... 🤔

I haven't yet sorted the oddity of an argument conversion (which may be perfectly natural, according to recent discussion about marking a conversion into the desired type), a conversion that is Any => Any albeit with a marker, the Show business (oops, pun) that is not a typeclass, in particular that everything "is a" Showable and implements
def toText(printer: Printer): Text = printer.toText(this)
so the question about whether to just require a Context to print anything seems germane: to print my Foo, shouldn't I just supply a context with a specialized FooPrinter?

It feels like there is an extra degree of indirection from Any -> Shown -> Show -> Showable -> Printer. If I make a Foo, I'm happy to follow the instructions and also make a FooPrinter so it looks nice in debug output. I guess also in error messages. I think I saw on a ticket that those are different use cases.

I don't see your full picture. What a FooPrinter and where does it live? And what stays and what goes of the current daisy chain?

dwijnand · 2022-02-24T12:02:51Z

Thanks for trying it out and the review, @nicolasstucki!

dwijnand · 2022-03-23T08:53:44Z

This was failing in CI because a change in main had a bug that the safer setup in this branch caught. More reason to merge this, IMO, hint-hint.

As it was our string interpolators were taking Any values and then trying to pattern match back their classes to try to show them nicely. This inevitably fails for things like the opaque type FlagSet, which interpolates as an indecipherable long number. Now, instead, they take "Shown" arguments, for which there is an implicit conversion in scope, given there is a Show instance for value. I captured some desired results in some new unit test cases. In the process a small handful of bugs were discovered, the only particularly bad one was consuming a Iterator when the "transforms" printer was enabled (accessorDefs), followed by an unintentional eta-expansion of a method with a defaulted argument (showSummary). I also lifted out the Showable and exception fallback function as an extension method, so I could use it more broadly. The use of WrappedResult and its `result` in `showing` was also impacted, because the new expected Shown type was driving `result`'s context lookup. Fortunately I was able to continue to use WrappedResult and `result` as defined by handling this API change inside `showing` alone. I wasn't, however, able to find a solution to the impact the new Shown expected type was having on the `stripModuleClassSuffix` extension method, sadly. JSExportsGen is interpolating a private type, so rather than open its access or giving it a Show instance I changed the string interpolation. SyntaxFormatter was no longer used, since the "hl" interpolator was dropped.

This removes the need for some of the default instances. More importantly, this reduces the burden and boilerplate that would be needed when introducing a new case class, without defining a Show instance for it or extending Showable.

I added this at the last minute as a fallback, but I can't remember the use case exactly, and it muddies the waters by mixing concerns. Also other typo fixes and a code tweak.

It was also being used over TypeComparer.ApproxState's show extension method (for unclear reasons), so let's avoid name confusion too.

Another win for Show/Shown over Any.

compiler/src/dotty/tools/dotc/printing/Formatting.scala

nicolasstucki

I will re-run the CI to make sure we don't have conflicts with new code.

dwijnand marked this pull request as ready for review February 11, 2022 18:45

dwijnand requested a review from smarter February 11, 2022 18:46

dwijnand assigned smarter Feb 11, 2022

smarter removed their assignment Feb 17, 2022

dwijnand requested review from nicolasstucki and removed request for smarter February 17, 2022 13:21

dwijnand assigned nicolasstucki Feb 17, 2022

dwijnand force-pushed the shown branch from 587f89d to efe0db1 Compare February 23, 2022 23:00

som-snytt reviewed Feb 24, 2022

View reviewed changes

compiler/src/dotty/tools/dotc/printing/Formatting.scala Outdated Show resolved Hide resolved

som-snytt reviewed Feb 24, 2022

View reviewed changes

compiler/src/dotty/tools/dotc/core/Decorators.scala Outdated Show resolved Hide resolved

nicolasstucki suggested changes Feb 24, 2022

View reviewed changes

nicolasstucki assigned dwijnand and unassigned nicolasstucki Feb 24, 2022

dwijnand assigned nicolasstucki and unassigned dwijnand Feb 24, 2022

dwijnand requested a review from nicolasstucki February 24, 2022 12:06

dwijnand force-pushed the shown branch from 42b608b to 2afe70a Compare March 15, 2022 08:59

dwijnand force-pushed the shown branch from 2afe70a to ded6942 Compare March 23, 2022 08:53

dwijnand added 3 commits March 31, 2022 14:34

Define a low-priority instance of Show for Product

382af94

This removes the need for some of the default instances. More importantly, this reduces the burden and boilerplate that would be needed when introducing a new case class, without defining a Show instance for it or extending Showable.

Drop Show from Any#show extension method

0ce7c52

I added this at the last minute as a fallback, but I can't remember the use case exactly, and it muddies the waters by mixing concerns. Also other typo fixes and a code tweak.

dwijnand added 3 commits March 31, 2022 14:34

Rename to Any#tryToShow as it's not Show-based

480eb03

It was also being used over TypeComparer.ApproxState's show extension method (for unclear reasons), so let's avoid name confusion too.

Fix: use the value not the eta-expanded method!

579e14b

Another win for Show/Shown over Any.

Fixes now that explicit nulls are on

e5ed401

dwijnand force-pushed the shown branch from ded6942 to e5ed401 Compare March 31, 2022 13:00

dwijnand commented Apr 1, 2022

View reviewed changes

compiler/src/dotty/tools/dotc/printing/Formatting.scala Show resolved Hide resolved

nicolasstucki approved these changes Apr 11, 2022

View reviewed changes

nicolasstucki enabled auto-merge April 11, 2022 12:45

nicolasstucki merged commit eb6aaa0 into scala:main Apr 14, 2022

dwijnand deleted the shown branch April 14, 2022 10:29

nicolasstucki mentioned this pull request Apr 14, 2022

3.1.3-RC2 is not able to compile scala3-compiler from current main #14938

Closed

Kordyjan added this to the 3.2.0 milestone Aug 1, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Switch our string interpolators to use Show/Shown #14455

Switch our string interpolators to use Show/Shown #14455

dwijnand commented Feb 11, 2022 •

edited

Loading

smarter commented Feb 17, 2022

dwijnand commented Feb 17, 2022

dwijnand commented Feb 23, 2022

som-snytt commented Feb 24, 2022

som-snytt commented Feb 24, 2022

nicolasstucki commented Feb 24, 2022

nicolasstucki Feb 24, 2022

dwijnand Feb 24, 2022 •

edited

Loading

dwijnand Feb 24, 2022 •

edited

Loading

dwijnand Feb 24, 2022

dwijnand Feb 24, 2022

dwijnand commented Feb 24, 2022

dwijnand commented Feb 24, 2022

dwijnand commented Mar 23, 2022

nicolasstucki left a comment

Switch our string interpolators to use Show/Shown #14455

Switch our string interpolators to use Show/Shown #14455

Conversation

dwijnand commented Feb 11, 2022 • edited Loading

smarter commented Feb 17, 2022

dwijnand commented Feb 17, 2022

dwijnand commented Feb 23, 2022

som-snytt commented Feb 24, 2022

som-snytt commented Feb 24, 2022

nicolasstucki commented Feb 24, 2022

nicolasstucki Feb 24, 2022

Choose a reason for hiding this comment

dwijnand Feb 24, 2022 • edited Loading

Choose a reason for hiding this comment

dwijnand Feb 24, 2022 • edited Loading

Choose a reason for hiding this comment

dwijnand Feb 24, 2022

Choose a reason for hiding this comment

dwijnand Feb 24, 2022

Choose a reason for hiding this comment

dwijnand commented Feb 24, 2022

dwijnand commented Feb 24, 2022

dwijnand commented Mar 23, 2022

nicolasstucki left a comment

Choose a reason for hiding this comment

dwijnand commented Feb 11, 2022 •

edited

Loading

dwijnand Feb 24, 2022 •

edited

Loading

dwijnand Feb 24, 2022 •

edited

Loading